Skip to content

Document the landing-pad cleanup-marker contract; close out review item 8#235

Merged
ASDAlexander77 merged 1 commit into
mainfrom
docs/landing-pad-marker-contract
Jul 14, 2026
Merged

Document the landing-pad cleanup-marker contract; close out review item 8#235
ASDAlexander77 merged 1 commit into
mainfrom
docs/landing-pad-marker-contract

Conversation

@ASDAlexander77

Copy link
Copy Markdown
Owner

Summary

Closes the last open finding (item 8) of docs/LowerToAffineLoops-LowerToLLVM-review.md — resolved as not a defect after tracing the full cross-pass flow, rather than "fixed":

  • The suspicious clause in windows::LandingPadOpLowering's cleanup branch is not a typed-catch RTTI value (the original review misread it) — for cleanup pads it is always the undef-array marker built in TryOpLowering. Being array-typed, it becomes an LLVM filter clause: a deliberate cleanup marker consumed by the custom LLVM passes.
  • LandingPadFixPass runs unconditionally first in the LLVM pipeline (transform.cpp:323, before any optimization) and canonicalizes every filter-bearing landingpad to a clause-less landingpad { cleanup }; Win32ExceptionPass (Windows, second) then converts pads to SEH funclets and erases every landingpad. The optimizer/inliner runs only after both — the filter never reaches EH-table emission on either platform.
  • Verified empirically via --emit=llvm final-IR dumps of 00try_finally.ts: Windows triple → zero landingpad/filter instructions (all funclets: 24 catchpad / 12 cleanuppad / 13 catchswitch); Linux triple → zero filter, canonical clause-less cleanup pads, proper catch ptr @_ZTI... catch clauses.

Changes: rewrites the misleading self-flagged BUG/HACK comments at all three sites (marker construction in LowerToAffineLoops.cpp, filter emission in windows::LandingPadOpLowering, filter detection in Win32ExceptionPass::isCleanup()) into accurate contract documentation cross-referencing each other; renames the catch1Fake local to cleanupMarker; updates the review doc. No functional change.

With this, all 10 findings from the lowering-passes review are closed (9 fixed across #230/#232/#233/#234, 1 resolved as not-a-bug).

Test plan

  • ctest --test-dir __build/tslang/windows-msbuild-2026-debug -C Debug -j8 — 690/690 passed (JIT + AOT)
  • Final-IR verification on both x86_64-pc-windows-msvc and x86_64-pc-linux-gnu triples confirming the marker is fully consumed before codegen

…em 8

Item 8 of docs/LowerToAffineLoops-LowerToLLVM-review.md (the self-flagged
"BUG: in LLVM landing pad is not fully implemented" in the Windows
LandingPadOpLowering) is resolved as not-a-defect after tracing the full
cross-pass flow: the suspicious clause is the undef-array marker built in
TryOpLowering (never an RTTI pointer), which becomes an LLVM *filter*
clause and serves as a deliberate cleanup marker. LandingPadFixPass
(always first in the LLVM pipeline, before any optimization) canonicalizes
such pads to clause-less cleanup landingpads, and Win32ExceptionPass then
replaces all pads with SEH funclets -- verified empirically via final-IR
dumps on both Windows and Linux triples (zero filter clauses survive;
Windows IR has zero landingpads at all).

Rewrites the misleading BUG/HACK comments at all three sites (marker
construction, filter emission, filter detection) into accurate contract
documentation cross-referencing each other, and renames the catch1Fake
local to cleanupMarker. No functional change; full ctest suite green
(690/690, JIT + AOT).

With this, all 10 findings from the lowering-passes review are closed
(9 fixed, 1 resolved as not-a-bug).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ASDAlexander77 ASDAlexander77 merged commit 35347c1 into main Jul 14, 2026
2 checks passed
@ASDAlexander77 ASDAlexander77 deleted the docs/landing-pad-marker-contract branch July 14, 2026 23:37
ASDAlexander77 added a commit that referenced this pull request Jul 15, 2026
A follow-up static audit of LowerToAffineLoops.cpp, LowerToLLVM.cpp, and
the TypeScriptExceptionPass files (beyond the 10 findings already closed
in PRs #230/#232/#233/#234/#235) found and fixes 7 more confirmed bugs:

- SwitchStateOpLowering: generator yield/resume state labels were
  collected into a SmallPtrSet<Operation*,16>, whose iteration order is
  only insertion order below 16 entries; past that it silently falls back
  to pointer-hash order, scrambling the positional switch dispatch used
  by generator resume. Replaced with an order-preserving SmallVector.
- StringConcatOpLowering: the stack-allocation path sized its Alloca
  using a pointer type instead of i8, over-allocating by sizeof(ptr) for
  every multi-argument string concatenation (e.g. console.log with 2+
  args).
- ArraySpliceOpLowering: mixed genuine MLIR index-dialect ops with
  already-LLVM-converted operands, a type mismatch that made every
  Array.prototype.splice() call fail to lower correctly (the only
  .splice() call in the test suite was commented out because of this).
  Reworked to stay consistently in the LLVM-converted domain, matching
  every sibling array-mutation lowering.
- GlobalOpLowering: the side-effect enumeration deciding whether a global
  initializer can be inlined as a constant vs. must run as a real
  constructor was missing several ops with real side effects (array
  push/unshift/splice/pop/shift, delete, set-length, string concat/
  char-to-string).
- LandingPadFixPass: MadeChange was set unconditionally for every
  landingpad visited, even when nothing was rewritten, causing needless
  analysis invalidation on every EH-containing function on every compile.
- Win32ExceptionPass: the PHI-user removal loop erased only the first PHI
  user found then unconditionally erased the underlying value, violating
  LLVM's no-remaining-uses invariant when more than one PHI referenced the
  same to-be-removed value (own TODO comment flagged this as incomplete).
- Win32ExceptionPass: getThrowFn's fallback path created a stray
  _CxxThrowException Function without attaching it to the module.

Added 00array_splice.ts (JIT + AOT) covering shrink/grow/equal-size
splice cases, since this path had zero prior test coverage. Full ctest
suite green: 692/692.

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant